Block: split out toolbar rendering #19564

ellatrix · 2020-01-10T15:37:02Z

Description

Offers a 7% performance gain.

This PR moves the toolbar popover out of the block tree. Eventually, the goal is to be able to remove the special popover slot and maybe unify it with the fixed toolbar and/or mobile toolbar. I suspect it may improve performance a bit too.

This step is very important in simplifying the React tree of the block and further performance improvements.

Worth noting is that I rewrote the way toolbar capturing works. Since toolbars are in popovers, toolbar capturing is a matter of attaching the popover to the right parent block node. There's no need to use slots.

Also worth noting is that block nodes are added to the block-editor state. This is very useful for any components that need to render based on wether or not a block node in actually mounted in the DOM. I marked this API unstable for now.

How has this been tested?

Everything should work as before. Test toolbar rendering.

Screenshots

Types of changes

Refactoring.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

ellatrix · 2020-01-11T15:14:39Z

packages/block-editor/src/components/block-list/block.js

@@ -307,7 +274,6 @@ function BlockListBlock( {
 			'is-focused': isFocusMode && ( isSelected || isAncestorOfSelectedBlock ),
 			'is-focus-mode': isFocusMode,
 			'has-child-selected': isAncestorOfSelectedBlock,
-			'has-toolbar-captured': hasAncestorCapturingToolbars,


Removing this class since it was used only for one broken CSS rule. Additionally it's a lot of selectors running for each block.

ellatrix · 2020-01-11T15:16:27Z

packages/block-editor/src/components/block-list/style.scss

-
-	// Toolbar is captured
-	&.has-toolbar-captured::before {
-		left: 0; // place "selected" indicator closer to block due to no toolbar


This rule is targeting .block-editor-block-list__layout, but .has-toolbar-captured is only added to blocks, so it's broken. I don't see any issues with the block border without this rule. Everything is looking good. Additionally there are plans to get rid of the border altogether anyway.

ellatrix · 2020-01-11T19:38:15Z

Performance test:


   417a86729f..b140054e5c  try/move-toolbar-popover -> try/move-toolbar-popover
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (21.569s)
  Performance
    ✓ 1000 paragraphs (18586ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        21.68s, estimated 23s
Ran all test suites.

Average time to load: 4757ms
Average time to DOM content load: 4724ms
Average time to type character: 32.01ms
Slowest time to type character: 81ms
Fastest time to type character: 19ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout master
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (24.021s)
  Performance
    ✓ 1000 paragraphs (19472ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        24.177s
Ran all test suites.

Average time to load: 5017ms
Average time to DOM content load: 4984ms
Average time to type character: 34.385ms
Slowest time to type character: 109ms
Fastest time to type character: 21ms
		
Ellas-MacBook-Pro:gutenberg ella$

youknowriad · 2020-01-13T08:43:01Z

Also worth noting is that block nodes are added to the block-editor state. This is very useful for any components that need to render based on wether or not a block node in actually mounted in the DOM. I marked this API unstable for now.

In general we avoid using complex objects (DOM elements) in state. Any particular reason to avoid block DOM utils.

youknowriad · 2020-01-13T08:45:23Z

packages/block-editor/src/components/block-list/block.js

@@ -564,16 +420,13 @@ const applyWithSelect = withSelect(
 			isValid,
 			isSelected,
 			isAncestorOfSelectedBlock,
-			isCapturingDescendantToolbars,
-			hasAncestorCapturingToolbars,


I don't expect the removed props from this component to have been used by plugins (blockedit filter), that said, we need to communicate this with a dev note.

It was added recently, so it won't be in any WP release. #18440

ellatrix · 2020-01-13T09:03:36Z

In general we avoid using complex objects (DOM elements) in state. Any particular reason to avoid block DOM utils.

Yes, we need the component to rerender when the block DOM node is mounted, but I can simplify it by only saving the client ID and then getting the element by ID. I'll adjust it.

ellatrix · 2020-01-13T12:58:11Z

Let's merge and iterate as this is a good separation to have and it has clear performance benefits.

youknowriad · 2020-01-13T13:07:13Z

packages/block-editor/src/store/reducer.js

+ *
+ * @return {boolean} Updated state.
+ */
+export function selectedMountedBlock( state, action ) {


How is this different from the block selection state?

It's only set when the block node is mounted. This is useful when you have to render a popover at the position of this node. We can't render the popover before the node is mounted.

I honestly feel this is entirely useless. If the selected block is in the store, it's already mounted right? at worst it's mounted after a useEffect

I don't think it's useless, but we can try alternative ways for sure. Being a selected block in the store doesn't mean that the block is mounted. React first has to render the block. This is similar to the block node not being available at the time of rendering. You have to use useEffect.

I'm open to alternatives though.

I feel having a state just to say "something is mounted" is not great. ideally, something in the state should make the block mount, in the case of blocks, blocks are mounted as soon as they are in the state, which for me suggests that it should be treated at the component level.

I guess the main issue here is that the rendering of the block and the rendering of the toolbar arae separate in two separate React trees so for the first render of the blocks, we're not certain which one will render first.

I'm not sure what's the perfect solution here other than just using an interval/useEffect or something like that.

Right, the only dependency on BlockListBlock is the position of the DOM node. I totally agree with you that it's not ideal and that's why I marked it unstable. I'll explore an alternative idea in a separate PR.

We could try to provide Popover with a selector for the anchor. Popover already repositions at an interval, so this could work. Additionally, we also already use a selector for multi selected block (to enable sticking the toolbar).

ellatrix added the [Status] In Progress Tracking issues with work in progress label Jan 10, 2020

ellatrix force-pushed the try/move-toolbar-popover branch 3 times, most recently from e592896 to aa9195a Compare January 10, 2020 23:51

ellatrix added 2 commits January 11, 2020 02:13

wip

7644c11

Wait for block to mount

43227f6

ellatrix force-pushed the try/move-toolbar-popover branch from d9b41e1 to 43227f6 Compare January 11, 2020 01:15

ellatrix added 5 commits January 11, 2020 02:53

Fix some tests

e737109

Revert moving focus effects

7aae315

Fix align error

15572f8

Fix navigation mode

327c04b

Fix capturing toolbar

8f7eb28

ellatrix force-pushed the try/move-toolbar-popover branch from 9d5ac15 to 8f7eb28 Compare January 11, 2020 15:01

Remove has-toolbar-captured

9f4d0fe

ellatrix marked this pull request as ready for review January 11, 2020 15:11

ellatrix requested review from talldan and youknowriad as code owners January 11, 2020 15:11

ellatrix changed the title ~~[WIP] Block: split out toolbar rendering~~ Block: split out toolbar rendering Jan 11, 2020

ellatrix added [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts and removed [Status] In Progress Tracking issues with work in progress labels Jan 11, 2020

ellatrix commented Jan 11, 2020

View reviewed changes

ellatrix added 7 commits January 11, 2020 16:35

Add docs

93884bc

Simplify multi selection selector

809024f

Fix alignment

7206b8a

Clean up

b5713b3

Only add block node for selected block

2ce59a8

Simplify block DOM node state

7dda5cb

Fix alignment tests

417a867

Clean up

b140054

Clean up

16eb079

youknowriad reviewed Jan 13, 2020

View reviewed changes

youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 13, 2020

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 13, 2020

ellatrix force-pushed the try/move-toolbar-popover branch from 881b5d3 to 35745da Compare January 13, 2020 09:54

Avoid node in state

dcd76e2

ellatrix force-pushed the try/move-toolbar-popover branch from 35745da to dcd76e2 Compare January 13, 2020 10:19

ellatrix merged commit c3f7820 into master Jan 13, 2020

ellatrix deleted the try/move-toolbar-popover branch January 13, 2020 12:58

youknowriad reviewed Jan 13, 2020

View reviewed changes

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

ellatrix mentioned this pull request Jan 21, 2020

Block: use context to provide selected element #19782

Merged

6 tasks

aduth mentioned this pull request Jan 27, 2020

Block Editor: Consider events only from DOM descendents (WritingFlow, ObserveTyping) #19879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block: split out toolbar rendering #19564

Block: split out toolbar rendering #19564

ellatrix commented Jan 10, 2020 •

edited

Loading

ellatrix Jan 11, 2020

ellatrix Jan 11, 2020

ellatrix commented Jan 11, 2020

youknowriad commented Jan 13, 2020

youknowriad Jan 13, 2020

ellatrix Jan 13, 2020

ellatrix commented Jan 13, 2020

ellatrix commented Jan 13, 2020

youknowriad Jan 13, 2020

ellatrix Jan 13, 2020

youknowriad Jan 13, 2020 •

edited

Loading

ellatrix Jan 13, 2020

ellatrix Jan 13, 2020

youknowriad Jan 13, 2020

ellatrix Jan 13, 2020

ellatrix Jan 13, 2020

Block: split out toolbar rendering #19564

Block: split out toolbar rendering #19564

Conversation

ellatrix commented Jan 10, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jan 11, 2020

youknowriad commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jan 13, 2020

ellatrix commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jan 10, 2020 •

edited

Loading

youknowriad Jan 13, 2020 •

edited

Loading